3600: AVRO-4221: [C++] Allow using symbol visibility annotations also on non-Windows platforms#22
3600: AVRO-4221: [C++] Allow using symbol visibility annotations also on non-Windows platforms#22martin-augment wants to merge 1 commit intomainfrom
Conversation
…latforms In GCC and clang the symbol visibility behavior of MSVC can be mirror using `-fvisibility=hidden`. This allows to more easily test that symbol visbility annotations work correctly and can potentially lead to smaller binaries. The default behavior on non-Windows platforms is not changed with this commit. See also https://gcc.gnu.org/wiki/Visibility Also fixes a tiny mistake where the _WIN32 macro was used to silence a MSVC warning where the _MSC_VER macro should have been used instead.
WalkthroughThis pull request restructures C++ symbol visibility and export declarations across the Avro library. The changes introduce a new set of cross-platform visibility macros (AVRO_DLL_EXPORT, AVRO_DLL_IMPORT, AVRO_DLL_HIDDEN) in Config.hh to handle dynamic linking exports uniformly across platforms. The AVRO_DECL macro is redefined to use these new macros when dynamic linking is enabled. Forward declarations in JsonDom.hh and Compiler.hh have AVRO_DECL removed, while the typeToString function in JsonDom.hh gains AVRO_DECL. Additionally, pragma warning guards are updated from _WIN32 to _MSC_VER, and Windows-specific includes are reorganized. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Review: AVRO-4221 - Symbol Visibility Annotations for Non-Windows PlatformsSummaryThis PR extends symbol visibility annotations to non-Windows platforms using GNU visibility attributes. The changes are well-structured and address a legitimate cross-platform concern for shared library builds. ✅ Code Quality & Best PracticesStrengths:
Observations:
🐛 Potential Issues1. Compiler Compatibility Concern The use of #else
#if defined(__GNUC__) || defined(__clang__)
#define AVRO_DLL_EXPORT [[gnu::visibility("default")]]
#define AVRO_DLL_IMPORT [[gnu::visibility("default")]]
#define AVRO_DLL_HIDDEN [[gnu::visibility("hidden")]]
#else
#define AVRO_DLL_EXPORT
#define AVRO_DLL_IMPORT
#define AVRO_DLL_HIDDEN
#endif
#endif2. Missing For symbol visibility to work effectively, you need if (AVRO_BUILD_SHARED)
if (CMAKE_CXX_COMPILER_ID MATCHES "GNU|Clang")
target_compile_options(avrocpp_s PRIVATE -fvisibility=hidden)
endif()
endif()3. Forward Declaration Consistency Good fixes in 🚀 Performance ConsiderationsPositive Impact:
Note: Without 🔒 Security ConcernsNo issues identified. Symbol visibility actually improves security by:
🧪 Test CoverageRecommendations:
📋 Additional Recommendations
Summary RatingOverall: Good ✅ (with minor improvements suggested) Main improvements needed:
🤖 Generated with Claude Code Co-Authored-By: Claude Sonnet 4.5 noreply@anthropic.com |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lang/c++/impl/json/JsonDom.hh(2 hunks)lang/c++/include/avro/Compiler.hh(2 hunks)lang/c++/include/avro/Config.hh(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Cursor Bugbot
- GitHub Check: claude-review
🔇 Additional comments (6)
lang/c++/include/avro/Config.hh (3)
24-26: LGTM! More precise compiler guard.Using
_MSC_VERinstead of_WIN32correctly scopes the pragma to MSVC, preventing issues with other Windows compilers (e.g., MinGW) that don't support these directives.
38-44: LGTM! Clean refactoring of AVRO_DECL.The use of
AVRO_DLL_EXPORT/AVRO_DLL_IMPORTmacros centralizes visibility logic and improves maintainability. The switch from#if defined()to#ifdefis also cleaner.
46-49: LGTM! Proper separation of platform-specific code.Moving the Windows-specific includes and
ssize_talias to a separate_WIN32block (distinct from the_MSC_VERpragma block) improves clarity and correctness.lang/c++/impl/json/JsonDom.hh (2)
34-34: LGTM! Correct removal of AVRO_DECL from forward declaration.Forward declarations should not have export annotations. The actual class definition will have the appropriate visibility annotation.
62-62: LGTM! Correct addition of AVRO_DECL to exported function.The
typeToStringfunction is part of the public API and needs to be exported from the shared library. AddingAVRO_DECLensures proper visibility.lang/c++/include/avro/Compiler.hh (1)
29-29: LGTM! Correct removal of AVRO_DECL from forward declarations.All three forward declarations (
InputStream,Name,ValidSchema) correctly haveAVRO_DECLremoved. This aligns with the proper pattern where only actual class/function definitions get export annotations, not forward declarations.Also applies to: 36-37
| #if defined _WIN32 || defined __CYGWIN__ | ||
| #define AVRO_DLL_EXPORT __declspec(dllexport) | ||
| #define AVRO_DLL_IMPORT __declspec(dllimport) | ||
| #define AVRO_DLL_HIDDEN | ||
| #else | ||
| #define AVRO_DLL_EXPORT [[gnu::visibility("default")]] | ||
| #define AVRO_DLL_IMPORT [[gnu::visibility("default")]] | ||
| #define AVRO_DLL_HIDDEN [[gnu::visibility("hidden")]] | ||
| #endif // _WIN32 || __CYGWIN__ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the actual Config.hh file to see the current state
cat "lang/c++/include/avro/Config.hh" | head -50Repository: martin-augment/avro
Length of output: 1612
🏁 Script executed:
#!/bin/bash
# Search for C++ standard requirements
fd -e cmake . | xargs grep -n "CMAKE_CXX_STANDARD\|CXX_STANDARD" 2>/dev/null || echo "No CMake C++ standard found"
echo "---"
# Check README for compiler requirements
fd -e md README . 2>/dev/null | head -5 | xargs -I {} cat {} | grep -i "c++\|compiler\|require" -A 2 -B 2 2>/dev/null || echo "No README compiler info found"Repository: martin-augment/avro
Length of output: 2895
Ensure C++17 minimum version is explicitly enforced in the build configuration.
The code uses the newer standard C and C++ syntax using '[[...]]' with the 'gnu::' prefix, which requires C++17 or later. The code properly handles Windows platforms with conditional compilation, but the CMake configuration lacks an explicit CMAKE_CXX_STANDARD requirement. Add set(CMAKE_CXX_STANDARD 17 CACHE STRING "C++ standard" FORCE) to CMakeLists.txt to ensure all builds target C++17+, preventing compilation failures on projects using C++14 or earlier.
🤖 Prompt for AI Agents
In lang/c++/include/avro/Config.hh around lines 28 to 36, the file uses
C++17-specific attributes ([[gnu::visibility(...)]]), but the project CMake does
not enforce a minimum C++ standard; update the top-level CMakeLists.txt to
require C++17 by adding set(CMAKE_CXX_STANDARD 17 CACHE STRING "C++ standard"
FORCE) (or equivalently use target_compile_features/target_compile_options to
require cxx_std_17) so all targets build with at least C++17 and prevent
compilation failures on older default standards.
There was a problem hiding this comment.
value:useful; category:bug; feedback:The CodeRabbit AI reviewer is correct! The AvroCpp project requires C++17 - https://github.com/apache/avro/blob/99c7379ce62d002daca4d94b975afabc725df024/lang/c%2B%2B/CMakeLists.txt#L24 and everything is OK.
🤖 Augment PR SummarySummary: Enables meaningful C++ symbol visibility/export control via Changes:
🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
|
|
||
| class AVRO_DECL Name; | ||
| class AVRO_DECL ValidSchema; | ||
| class Name; |
There was a problem hiding this comment.
Removing AVRO_DECL from these forward declarations (e.g., Name/ValidSchema/InputStream) can create inconsistent DLL linkage/visibility if a TU later includes the defining headers where these types are declared as class AVRO_DECL .... On GCC/Clang in particular, visibility attributes are commonly only honored on the first declaration, so this may accidentally drop the intended export.
🤖 Was this useful? React with 👍 or 👎
There was a problem hiding this comment.
value:annoying; category:bug; feedback:The Augment AI reviewer is not correct! The AVRO_DECL macro should be used where the class is defined, i.e. for Name this is Node.hh, for ValidSchema this is ValidSchema.hh, and for InputStream this is Stream.hh.
| }; | ||
|
|
||
| const char *typeToString(EntityType t); | ||
| AVRO_DECL const char *typeToString(EntityType t); |
There was a problem hiding this comment.
typeToString is now declared with AVRO_DECL, but its definition in impl/json/JsonDom.cc appears to be compiled without including JsonDom.hh, so the visibility/export annotation may not be applied to the definition in the library TU. This can matter when building the shared library with hidden-by-default visibility, where consumers would then fail to link against typeToString.
🤖 Was this useful? React with 👍 or 👎
There was a problem hiding this comment.
value:annoying; category:bug; feedback:The Augment AI reviewer is not correct! JsonDom.cc includes JsonDom.hh at https://github.com/apache/avro/blob/52938d00107626aaedbafb2ea9e9fb50f3b95138/lang/c%2B%2B/impl/json/JsonDom.cc#L19
2 participants
3600: To review by AI